Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update NJsonSchema to v11 #2263

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Update NJsonSchema to v11 #2263

merged 4 commits into from
Aug 26, 2024

Conversation

NoahStolk
Copy link
Contributor

(Original PR: #2185)

Fixes #2169

A few notes:

  • This is a breaking change, and should be included in a major version release only, as we're dropping support for NJsonSchema v10. This change will impact users that manage their own references to NJsonSchema v10.
  • Newtonsoft.Json-based generators have been moved to a separate NuGet package, so the reference to NJsonSchema has been replaced with a reference to NJsonSchema.NewtonsoftJson. See NSwag 14 announcement and NJsonSchema v11 release notes.
  • EnumHandling no longer exists, so the WithJsonSchemaGeneratorSettingsSerDe unit test has been updated to use JsonSerializerSettings instead.

@NoahStolk NoahStolk requested review from a team as code owners July 16, 2024 08:45
@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jul 16, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jul 16, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Noah Stolk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

This was referenced Jul 16, 2024
Closed
Closed
rayokota
rayokota approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@rayokota rayokota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NoahStolk , LGTM

@rayokota
Copy link
Member

@NoahStolk , I intend to merge this PR, and then we can figure out a subsequent PR to figure out how best to minimize breaking backward compatibility. Unfortunately I can't merge at the moment as we're doing some internal work to change the protection rules for public repos.

@rayokota
Copy link
Member

@NoahStolk , just an FYI that we don't have an ETA for when the protection rules will be fixed (hopefully in a few weeks), but I won't forget about this PR.

@NoahStolk
Copy link
Contributor Author

Thanks @rayokota, do you plan to merge this PR as is, and then add the NET8_0_OR_GREATER directives? Or should they be included in this branch before the merge?

@rayokota
Copy link
Member

If you want to add it to this PR, that would be great. Otherwise we could add it to a subsequent PR

@rayokota
Copy link
Member

rayokota commented Aug 9, 2024

@NoahStolk , on second thought, can you take a look at adding the directives to this PR? So that we don't break users in case we need to cut a patch release from master. Thanks!

@NoahStolk
Copy link
Contributor Author

Will try to find some time to do that this week.

rayokota
rayokota approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@rayokota rayokota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NoahStolk , LGTM

@NoahStolk
Copy link
Contributor Author

Thanks @rayokota

FYI: Not all (.NET 8) unit tests are passing yet, I'm getting some of these errors.

image

Any ideas? If not I'll debug some time later. Haven't had a lot of time to look into this yet.

I also don't have .NET 6 installed anymore, so I don't know if the old tests are still passing.

I had another question... Do we need to multi-target the JsonWithReferences and Confluent.SchemaRegistry.Serdes.IntegrationTests projects? I just upgraded them to .NET 8.

@rayokota
Copy link
Member

Thanks @NoahStolk , I believe the .NET 6 tests are still passing. Yeah, I'm not sure about the .NET 8 ones. Let me know if you can't figure it out and maybe I can take a look as well.

@rayokota
Copy link
Member

Thanks @NoahStolk. The issue is RicoSuter/NJsonSchema#1696. I'll merge this PR and fix the tests in a subsequent PR.

@rayokota rayokota merged commit ef89a01 into confluentinc:master Aug 26, 2024
1 check passed
rayokota added a commit that referenced this pull request Aug 30, 2024
@rayokota
Temporarily revert NJsonSchema v11 changes (#2302)
c140324 
* Revert "Fix tests for NJsonSchema 11 (#19) (#2291)"

This reverts commit bebc802.

* Revert "Update NJsonSchema to v11 (#2263)"

This reverts commit ef89a01.
@NoahStolk
Copy link
Contributor Author

Thank you!

@NoahStolk NoahStolk deleted the update-NJsonSchema-to-v11 branch September 4, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rayokota rayokota rayokota approved these changes

Assignees
No one assigned
Labels
schema registry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Binary incompatibility with NJsonSchema version 11.0.0
2 participants
@NoahStolk @rayokota